Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

colexec: add support for bit and some arithmetic binary operators #49761

Merged
merged 2 commits into from
Jun 5, 2020
Merged

colexec: add support for bit and some arithmetic binary operators #49761

merged 2 commits into from
Jun 5, 2020
+303 −103

Conversation

yuzefovich
Copy link
Member

sem: unify division by zero check and fix it in a few places

Release note (bug fix): Previously, in some cases, CockroachDB didn't
check whether the right argument of Div (/), FloorDiv (//),
or Mod (%) operations was zero, so instead of correctly returning
a "division by zero" error, we were returning NaN, and this is now
fixed. Additionally, the error message of "modulus by zero" has been
changed to "division by zero" to be inline with Postgres.

colexec: add support for bit and some arithmetic binary operators

This commit adds support for Bitand, Bitor, Bitxor, FloorDiv,
and Mod binary operators for both native and datum-backed types.

Release note (sql change): Vectorized execution engine now supports
Bitand (&), Bitor (|), Bitxor (^), FloorDiv (//), and
Mod (%) binary operators.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich
Copy link
Member Author

These are the third and fourth commit that were extracted from #49419.

@yuzefovich yuzefovich changed the title More datum bin overloads colexec: add support for bit and some arithmetic binary operators Jun 1, 2020
@yuzefovich yuzefovich mentioned this pull request Jun 1, 2020
Closed
9 tasks
@asubiotto
Copy link
Contributor

I gave this an LGTM in the other PR so leaving to @jordanlewis to review.

@yuzefovich yuzefovich force-pushed the more-datum-bin-overloads branch 3 times, most recently from 1a76721 to 18e91f5 Compare June 4, 2020 23:56
@yuzefovich
Copy link
Member Author

@jordanlewis I'm assuming you want to take a look at this when you get a chance (it's not urgent), right?

@jordanlewis
Copy link
Member

Sorry, I've been behind in the reviews last couple of days.

@yuzefovich
Copy link
Member Author

No worries, I understand, just checking that I should wait before merging this.

jordanlewis
jordanlewis approved these changes Jun 5, 2020
View reviewed changes
Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @yuzefovich)

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 257 at r1 (raw file):

				{{if .CheckRightIsZero}}
				if {{.Right}} == 0.0 {
					colexecerror.ExpectedError(tree.ErrDivByZero)

LGTM. As a (non-blocking) idea, would it make sense to introduce a new operator in the chain before division operators, that returns the error if any divisor is 0? I'm not positive, but based on lessons we've learned from vectorized, it seems at least likely that it would be more efficient to do the check in a single pass.

@yuzefovich
sem: unify division by zero check and fix it in a few places
b973a82 
Release note (bug fix): Previously, in some cases, CockroachDB didn't
check whether the right argument of `Div` (`/`), `FloorDiv` (`//`),
or `Mod` (`%`) operations was zero, so instead of correctly returning
a "division by zero" error, we were returning `NaN`, and this is now
fixed. Additionally, the error message of "modulus by zero" has been
changed to "division by zero" to be inline with Postgres.
yuzefovich
yuzefovich commented Jun 5, 2020
View reviewed changes
Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 257 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

LGTM. As a (non-blocking) idea, would it make sense to introduce a new operator in the chain before division operators, that returns the error if any divisor is 0? I'm not positive, but based on lessons we've learned from vectorized, it seems at least likely that it would be more efficient to do the check in a single pass.

Oh, that's an interesting idea. I agree that it is likely to be more performant, left a TODO for this.

@yuzefovich
Copy link
Member Author

bors r-

A minor typo in the comment.

@craig
Copy link
Contributor

craig bot commented Jun 5, 2020

Canceled

@yuzefovich
colexec: add support for bit and some arithmetic binary operators
ca6c664 
This commit adds support for `Bitand`, `Bitor`, `Bitxor`, `FloorDiv`,
and `Mod` binary operators for both native and datum-backed types.

Release note (sql change): Vectorized execution engine now supports
`Bitand` (`&`), `Bitor` (`|`), `Bitxor` (`^`), `FloorDiv` (`//`), and
`Mod` (`%`) binary operators.
@yuzefovich
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 5, 2020

Build succeeded

@craig craig bot merged commit 432a1c5 into cockroachdb:master Jun 5, 2020
@yuzefovich yuzefovich deleted the more-datum-bin-overloads branch June 5, 2020 16:52
@tancredosouza
Copy link
Contributor

Hello there!
First of all, thank you so much for sharing your PR as guidance. It has been of great help.
I've spent the last couple of days looking through it, trying to answer some questions I'm having.

I would like to ask one I wasn't able to answer on my own.
In the code translated from tree/eval.go to the getBinOpAssignFunc methods, how did you deal with operands of types.VarBit and types.INet?

Thanks!

@yuzefovich
Copy link
Member Author

First of all, thank you so much for sharing your PR as guidance. It has been of great help.

You're welcome @tancredosouza , thanks for looking into this!

In the code translated from tree/eval.go to the getBinOpAssignFunc methods, how did you deal with operands of types.VarBit and types.INet?

I think what might be confusing is that we have two kinds of representations for different types. For some types (like types.Int, types.Float) we have a "native" physical representation (like int64, float64) that is optimized for the particular type.

But for others (like VarBit and INet) we don't yet have such "native" representation, and we use tree.Datum-backed vector (coldataext.datumVec) to support all of the unoptimized types. You can think of these non-natively represented types as being supported by a single "generic" vector implementation.

Furthermore, for optimized types we do the translation of the relevant code in tree/eval.go to be included in the type customizers so that the code is inlined during the code generation, but for the datum-backed types we end up reusing exactly the same tree.BinOp.Fn function from eval.go to compute the binary expression (the correct function is passed in in colexec/execplan.go). As a result, there is no "translation" needed for the types that are datum-backed, like VarBit or INet. Does this make sense?

@tancredosouza
Copy link
Contributor

It does! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordanlewis jordanlewis jordanlewis approved these changes

@asubiotto asubiotto Awaiting requested review from asubiotto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yuzefovich @cockroach-teamcity @asubiotto @jordanlewis @tancredosouza